💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1dd914a75a

Left a few comments. Main concern is the TOCTOU race in _handle_event — the check-then-act on self._instances without holding the lock can cause duplicate registrations under concurrent events. The rest are smaller cleanups (license headers, type annotations, hardcoded test ports).

PR #1465 Review: Add coordinator module and corresponding unit test

Overview

This PR implements the OmniCoordinator module for distributed stage instance coordination (Major PR 1 from #984). The implementation closely follows the design document with a few important deviations.

Critical Issues: 4 found

1. Instance Registry Never Removes DOWN Instances (Design Deviation)

File: omni_coordinator.py:271-276

The design document states instances should be "removed from the internal registry" when DOWN, but the implementation only marks them as DOWN and keeps them forever:

def _remove_instance_locked(self, event: InstanceEvent) -> None:
    info.status = StageStatus.DOWN  # Only marks, never removes

Impact: Memory leak, unbounded registry growth over time.

2. ZMQ Context Singleton Causes Test/Production Issues

Files: omni_coordinator.py:51, omni_coord_client_for_stage.py:27, omni_coord_client_for_hub.py:28

Using zmq.Context.instance() shares a global context across all components:

self._ctx = zmq.Context.instance()  # Global singleton

Impact: Tests interfere with each other, closing one coordinator can break others, cannot run multiple coordinators.

Recommendation: Use per-coordinator context: self._ctx = zmq.Context()

3. No Connection State Machine / Reconnection Logic

Files: omni_coord_client_for_stage.py, omni_coord_client_for_hub.py

The design mentions reliability with retry mechanisms, but there's no reconnection logic if the ZMQ connection drops. Network blips will cause silent failures with no recovery path.

Recommendation: Add connection state machine with exponential backoff reconnection.

4. Scalability Limit Not Documented

The single-threaded coordinator has inherent scalability limits:

Recommendation: Add max_instances parameter or document expected limits.

Strengths

• ✅ Good test coverage matching design spec
• ✅ Message schemas match design document exactly
• ✅ Clean separation of concerns (messages, coordinator, clients)
• ✅ Thread-safe with proper locking
• ✅ ZMQ best practices (RCVTIMEO, NOBLOCK)

Recommendation

Address issues #1 and #2 before merge. Issue #3 (reconnection) could be documented as Phase 2 work if needed.

All threads resolved LGTM